Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unneeded reserve() calls #391

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

rjoomen
Copy link
Contributor

@rjoomen rjoomen commented Mar 4, 2024

In expr_ops.hpp reserve() was called at every variable update. This causes the variable and coefficient vectors to be relocated very often, as their size is updated by a minimal amount every time . Removing the reserve() calls allows the vectors to use their normal growing behavior, improving performance.

Before:
image

After:
image

Copy link
Contributor

@Levi-Armstrong Levi-Armstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a shrink to fit call somewhere because I had memory issue in the past?

@rjoomen
Copy link
Contributor Author

rjoomen commented Mar 5, 2024

I'm not sure. Do you know what size the coeff and var vectors typically are? The normal automatic vector reserve growing behavior is by a fixed factor of 1.5, so in the worst case we only use 1.5 times the amount of memory compared to the current situation.

@Levi-Armstrong
Copy link
Contributor

I will run heaptrack in the near future.

@Levi-Armstrong Levi-Armstrong merged commit 47d8bf5 into tesseract-robotics:master Mar 5, 2024
8 of 10 checks passed
@rjoomen
Copy link
Contributor Author

rjoomen commented Mar 6, 2024

I just read that a shrink_to_fit can also cause a reallocation. So unless we really need the memory it's not advisable to use it.

@rjoomen rjoomen deleted the optimizations branch March 7, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants